New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[systemjs] Fix nested let
/const
shadowing imported bindings
#14057
[systemjs] Fix nested let
/const
shadowing imported bindings
#14057
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51497/ |
declar.parentPath.parentPath.isBlockStatement() && | ||
declar.parent.kind !== "var" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we can hoist this check outside of the for
loop:
const needsRename = path.node.kind !== "var" && path.parentPath.isBlockStatement()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think we can just edit the Scope
visitor above to check kind === "let" || kind === "const"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will change that.
@@ -7,4 +7,4 @@ expect( | |||
bar; | |||
} | |||
).toBe("foo"); | |||
expect(bar).toBe("foo"); | |||
// expect(bar).toBe("foo"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behavior should not change.
for (const declar of declarations) { | ||
firstId = declar.node.id; | ||
if (needsRename) { | ||
declar.scope.rename(declar.node.id.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TS warning here is correct; I think this breaks, for example, with const { x } = {}
. You probably need to do
if (needsRename) {
for (const name of Object.keys(declar.getBindingIdentifiers()) {
declar.scope.rename(name);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add a test for this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will add the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it's working now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
execute: function () { | ||
if (true) { | ||
({ | ||
x: _x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why const { x } = { x: 1 }
is also transformed? I would expect _x
is registered in the same scope of the local x
declaration:
if (true) {
const { x: _x } = { x: 1 };
console.log(_x);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hoisted vars are renamed, So I think that's the reason for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should we change in this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been thinking about this, and the proper fix probably is in the systemjs
plugin itself.
We should not hoist all variables (var
and let
/const
), but only var
and top-level let
/const
. We can do this by:
- In the big loop over the program body (in the
Program.exit
visitor), we convert all the top-levellet
/const
bindings tovar
. This is safe, they are not in nested blocks. - When calling
hoistVariables
, we should remove the third parameter (null
) so that it only hoistsvar
and not alsolet
/const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For (1), be careful because export let x = 1;
and export { x }; let x = 1;
are handled in two different code paths and both should still work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If third parameter is removed from the hoistVariable
it removes all the _export
calls from the outputs, and doesn't transform the input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we will still need to hoist these export bindings and set up the metadata from which the _export
calls are built. I suggest we copy the code from the hoistVariable
to the systemjs transform, and implement the custom hoisting logic mentioned in #14057 (comment). Or we can add a new option in the helper, though I lean to the first option since the hoisting logic here is not reused by other plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised that the current helper doesn't work, because we only need _export
for things that become var
in step 1 🤔
Do you mind pushing the current (non working) code you have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a working branch. We can infer the behaviour of hoistVariable
helper from the implementation:
babel/packages/babel-helper-hoist-variables/src/index.ts
Lines 23 to 32 in dacaab1
Scope(path: NodePath, state: State) { | |
if (state.kind === "let") path.skip(); | |
}, | |
FunctionParent(path: NodePath) { | |
path.skip(); | |
}, | |
VariableDeclaration(path: NodePath<t.VariableDeclaration>, state: State) { | |
if (state.kind && path.node.kind !== state.kind) return; |
The third variable kind
controls two behaviours: 1) Whether we should check a BlockParent to see if it contains to-be-hoisted variables, and 2) whether we should hoist the let
-kind variable declarations.
In systemjs transform, what we want to achieve is to hoist var
variables within block parent (so kind
has to be "let"
), but leave let
-kind declarations within block parent untouched.
- If we set
kind
to"var"
, onlyvar
-kind declarations will be hoisted, the top levellet
-kinds are not. - If we set
kind
tonull
, all declarations will be hoisted unless nested within FunctionParent. But we want to preservelet
within block parent - If we set
kind
to"let"
, onlylet
-kind declarations will be hoisted
Therefore we still need to extend the hoistVariable helper behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@The-x-Theorist I hope you don't mind, I pushed two commits to your branch.
14e9cfd
(#14057) is how I was thinking to fix the problem, as described in #14057 (comment):
- It changes all the top-level
let
/const
tovar
- It only hoists
var
s
The updated tests look good, but it probably needs a few new tests to make sure that both const x = 1; export { x }
and export const x = 1;
work (unless we already have these tests).
// Convert top-level variable declarations to "var", | ||
// because they must be hoisted | ||
declar.node.kind = "var"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A top level var declaration can be nested within for statement:
for (var x = 1;false;);
export { x }
or it could be within a do expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They already have var
, there is no need to manually change them.
@@ -38,8 +38,16 @@ const visitor = { | |||
> = path.get("declarations"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hoistVariable visitor should be merged with environmentVisitor
, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets do it in another PR since it's separate from this bug 👍
@The-x-Theorist I reverted your hoistVariable changes but I still kept your tests since they ensure that we fixed the bug. Thank you! |
let
/const
shadowing imported bindings
Variables will be renamed in the hoisting operation, and variables declared inside blockStatment will be different, hence they won't collide with any other identifier.